Reconcile cache with filesystem on get-projects so sessions stop going missing - #29
Conversation
JeanBaptisteRenard
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR @ymajoros — and for the two others! The diagnosis is spot on (get-projects only scanning the filesystem on cold start is exactly why sessions were vanishing), the reconcile approach is the right fix, and the test is well-structured. Much appreciated. 🙏
I ran the full suite locally on your branch: 102/102 pass. Two things before merge:
1. Please rebase onto current main (required)
Your branch is based on 9dd4a66, which predates 13219eb (fix(stats): wire replaceSessionMetrics + touchCachedModified into sessionCache ctx.db). On your base, the ctx.db literal in main.js is missing touchCachedModified and replaceSessionMetrics, which session-cache.js calls in refreshFolder's header-read fallback path. Calling undefined() there throws a TypeError that is silently swallowed by refreshFolder's try/catch — and your reconcile pass calls refreshFolder on every stale folder, so it widens the blast radius of that (now-fixed-on-main) bug.
A rebase onto current main picks up the wiring and resolves this — no code change needed on your side, just please double-check after rebasing that the sessionCache.init({ db: { ... } }) literal includes both functions.
(Context: this is a known pitfall on this fork — any db.js function used by session-cache.js must be whitelisted in the ctx.db literal in main.js, otherwise the worker fails silently.)
2. Reconcile gate misses offline subagent changes (should fix, or scope-exclude)
The gate compares getFolderIndexMtimeMs(folderPath) against meta.indexMtimeMs, but getFolderIndexMtimeMs only stats top-level *.jsonl — it doesn't descend into subagents/. A folder where only subagent transcripts changed while the app was closed won't be flagged, so those changes stay missing (the exact scenario this PR targets). Once a folder is flagged, refreshFolder correctly uses enumerateSessionFiles which does recurse — the gap is only in the gate condition.
Suggestion: extend getFolderIndexMtimeMs to also stat subagents/*.jsonl, and add a regression test for the subagent-only offline change (your test file structure makes that easy to add). If you'd rather keep the scope tight, a note in the PR description acknowledging the limitation works too, and we'll follow up.
Minor / non-blocking
loadProjectsin the renderer callsgetProjects(false)andgetProjects(true)viaPromise.all, so the reconcile pass runs twice per sidebar load. Harmless (it's cheap and sync), just worth a dedupe flag someday.
Verdict: request changes — but really just the rebase + a decision on the subagent gate. Happy to merge right after. Thanks again!
…g missing The session cache is only rebuilt from the filesystem on a cold start (populateCacheViaWorker runs only when the cache is completely empty). Once it has any rows, a project folder that changed while the app was closed — or that predates the build which first indexed it — is never re-scanned, so its sessions (and whole worktrees) silently disappear from the sidebar. The live fs.watch only covers changes while the app is running. When the cache is already populated, get-projects now reconciles first: the dead populateCacheFromFilesystem() (defined + exported, never called) becomes a stat-gated reconcileCacheFromFilesystem() that re-indexes only folders that are new or whose newest transcript is newer than cache_meta.indexMtimeMs — cheap when nothing changed. Also extend getFolderIndexMtimeMs to stat the full session-file set (enumerateSessionFiles), not just top-level *.jsonl, so a folder whose only offline change was a subagent transcript under <id>/subagents/ is still flagged for reconcile. Adds regression tests for the gate and the subagent-only case. Rebased onto current main, which wires touchCachedModified/replaceSessionMetrics into the ctx.db literal.
c1da4cb to
51924a2
Compare
|
Thanks for the thorough review! Addressed both blocking points — force-pushed. 1. Rebased onto current 2. Subagent gate. Good catch. 3. (non-blocking) Yep — |
JeanBaptisteRenard
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed version — both blocking points are cleanly addressed:
- ✅ Rebase picks up the
ctx.dbwiring (touchCachedModified+replaceSessionMetricsverified present in the literal) - ✅
getFolderIndexMtimeMsnow walksenumerateSessionFiles— same enumeration asrefreshFolder, so the gate covers exactly what a refresh would scan, subagents included. The regression test is exactly what I hoped for.
Local run: 121/121 tests pass, CI green on Node 20/22.
Merging. I'll push a tiny follow-up PR for the two non-blocking notes (a comment documenting the intentionally-await-less sync call, and a small throttle for the double sweep from loadProjects's twin get-projects calls) — no action needed on your side.
Thanks a lot for the quality of this one, and for the fast, thorough turnaround on the review. Sessions silently vanishing was one of the most annoying bugs on this fork — great to have it nailed. 🙏
Problem
Sessions and whole worktrees silently vanish from the sidebar while their transcripts are still on disk, and reopening/restarting doesn't bring them back.
get-projectsonly scans the filesystem on a cold start:Once the cache is non-empty it's never reconciled again, so folders that changed while the app was closed — or that were never indexed by the build that first populated the cache (
cache_meta.indexMtimeMs === 0) — never reappear. The livefs.watchonly covers changes while the app is running.Fix
When the cache is already populated, reconcile before building from cache. The dead
populateCacheFromFilesystem()(defined + exported, never called) is repurposed intoreconcileCacheFromFilesystem(), now stat-gated: it re-indexes only folders that are new or whose newest.jsonlis newer than what was last indexed.When nothing changed this is just a
readdir+statper folder — no transcript parsing, no DB writes — so steady-state cost is negligible; real work happens only for folders that need it.refreshFolderalready does its own per-file mtime skip, so this composes with the targeted-refresh path you added.Testing
test/reconcile-cache.test.jsasserting the gate: a never-indexed folder and a stale folder (indexMtimeMsolder than disk) get indexed; an up-to-date folder is skipped. Passes undernode --test test/reconcile-cache.test.js(also randerive-project-path+folder-index-stategreen). I didn't run thedom-*tests here (needjsdom/devDeps installed).indexMtimeMs = 0) re-indexed and reappeared.Notes
asyncget-projects(placed in theelseso it doesn't double-scan right after a cold populate).session_meta(names/stars/archive) or settings.